fix: `build --no-codegen` output file name error #14239

apainintheneck · 2024-01-15T00:06:55Z

This avoids errors based on output file name when the --no-codegen command is passed to crystal build since it shouldn't be necessary to specify an output file if no file will be created anyway.

$ crystal build --no-codegen  example/example.cr
Error: can't use `example` as output filename because it's a directory

This change means that the following lines of code get skipped when using --no-codegen.

crystal/src/compiler/crystal/command.cr

Lines 598 to 601 in bb96c53

    
           # Check if we'll overwrite the main source file 
        
           if !no_codegen && !run && first_filename == File.expand_path(output_filename) 
        
             error "compilation will overwrite source file '#{Crystal.relative_filename(first_filename)}', either change its extension to '.cr' or specify an output file with '-o'" 
        
           end

crystal/src/compiler/crystal/command.cr

Lines 611 to 613 in bb96c53

    
           if !no_codegen && !run && Dir.exists?(output_filename) 
        
             error "can't use `#{output_filename}` as output filename because it's a directory" 
        
           end

It will also means that we skip the combine rpaths step.

crystal/src/compiler/crystal/command.cr

Line 619 in bb96c53

combine_rpath = run && !no_codegen

This avoids errors based on output file name when the `--no-codegen` command is passed to `crystal build` since it shouldn't be necessary to specify an output file if no file will be created anyway. Example: $ crystal build --no-codegen example/example.cr Error: can't use `example` as output filename because it's a directory

straight-shoota · 2024-01-15T13:52:30Z

Hey @apainintheneck, thanks for picking this up! 👋

This PR looks all right, as in it should probably work. But I'm a bit concerned about the semantics and wondering if there couldn't be a better solution.
The reason is that no_codegen is an input parameter and it feels a bit sketchy to change its value somewhere down the line. It will probably work fine because all the relevant code that uses it as the original input parameter is in the initial setup that and that has already executed at this point. So it's not used for that anymore.

Yet, wie have some duplication with compiler.no_codegen which contains the configuration value. So I'm thinking maybe instead we could replace all references to no_codegen by compiler.no_codegen in the subsequent code after the option parser setup.
And set compiler.no_codegen = no_codegen at the top of the method.
Then there would be a clear separation: no_codegen is only responsible for the CLI configuration. compiler.no_codegen determines if codegen is actually enabled in the compiler.

Does this make sense to you?

apainintheneck · 2024-01-15T17:25:44Z

Yet, wie have some duplication with compiler.no_codegen which contains the configuration value. So I'm thinking maybe instead we could replace all references to no_codegen by compiler.no_codegen in the subsequent code after the option parser setup. And set compiler.no_codegen = no_codegen at the top of the method. Then there would be a clear separation: no_codegen is only responsible for the CLI configuration. compiler.no_codegen determines if codegen is actually enabled in the compiler.

Does this make sense to you?

This makes sense to me. It shouldn't change semantics like you said but it will make the code easier to reason about.

The idea here is to make the `#create_compiler` method easier to reason about by splitting up the responsibilities of the `no_codegen` input parameter which is used when building the options parser and the `compiler.no_codegen?` boolean value which is used when deciding how to compile the program.

apainintheneck marked this pull request as ready for review January 15, 2024 02:28

apainintheneck changed the title ~~fix: build --no-codegen output file name warning~~ fix: build --no-codegen output file name error Jan 15, 2024

Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Jan 15, 2024

straight-shoota approved these changes Jan 15, 2024

View reviewed changes

straight-shoota added topic:compiler:cli and removed topic:compiler labels Jan 15, 2024

HertzDevil approved these changes Feb 1, 2024

View reviewed changes

straight-shoota added this to the 1.12.0 milestone Feb 1, 2024

straight-shoota merged commit 3def4de into crystal-lang:master Feb 2, 2024
57 checks passed

BrewTestBot mentioned this pull request Apr 9, 2024

crystal 1.12.0 Homebrew/homebrew-core#168465

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `build --no-codegen` output file name error #14239

fix: `build --no-codegen` output file name error #14239

apainintheneck commented Jan 15, 2024

straight-shoota commented Jan 15, 2024 •

edited

Loading

apainintheneck commented Jan 15, 2024

	# Check if we'll overwrite the main source file
	if !no_codegen && !run && first_filename == File.expand_path(output_filename)
	error "compilation will overwrite source file '#{Crystal.relative_filename(first_filename)}', either change its extension to '.cr' or specify an output file with '-o'"
	end

	if !no_codegen && !run && Dir.exists?(output_filename)
	error "can't use `#{output_filename}` as output filename because it's a directory"
	end

fix: build --no-codegen output file name error #14239

fix: build --no-codegen output file name error #14239

Conversation

apainintheneck commented Jan 15, 2024

straight-shoota commented Jan 15, 2024 • edited Loading

apainintheneck commented Jan 15, 2024

fix: `build --no-codegen` output file name error #14239

fix: `build --no-codegen` output file name error #14239

straight-shoota commented Jan 15, 2024 •

edited

Loading